Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a history endpoint #45

Merged
merged 1 commit into from
Oct 19, 2018
Merged

Add a history endpoint #45

merged 1 commit into from
Oct 19, 2018

Conversation

invacuo
Copy link
Collaborator

@invacuo invacuo commented Oct 16, 2018

Notes:

  • Also decided to alphabetize the order in which we require the files in the spacex.rb file

@invacuo invacuo force-pushed the add_history_endpoint branch from 163aa69 to 4d331b4 Compare October 16, 2018 02:53
@invacuo
Add a history endpoint
401c11f 
- A very basic history endpoint as described in the issue rodolfobandeira#39
- Doesn't yet support all the filters that are described in the documentation provided here https://documenter.getpostman.com/view/2025350/RWaEzAiG#9f1dfdc0-fbe8-4ae5-9209-7f3d649a627c
- Allows retrieving all the historical events or a specific historical event by passing an id
- This closes rodolfobandeira#39 when merged in.

Notes:
- Also decided to alphabetize the order in which we `require` the files in the `spacex.rb` file
@invacuo invacuo force-pushed the add_history_endpoint branch from 4d331b4 to 401c11f Compare October 16, 2018 03:11
@coveralls
Copy link

Pull Request Test Coverage Report for Build 97

  • 36 of 36 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 93: 0.0%
Covered Lines: 781
Relevant Lines: 781
💛 - Coveralls

@rodolfobandeira
Copy link
Owner

Hey @invacuo, Sorry I couldn't review this yet. I'm gonna take a look tonight!

@rodolfobandeira rodolfobandeira self-assigned this Oct 18, 2018
rodolfobandeira
rodolfobandeira approved these changes Oct 19, 2018
View reviewed changes
Copy link
Owner

@rodolfobandeira rodolfobandeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with it. Feel free to merge it.
Also, I'm gonna think on some ideas to implement all these filters and will let you know.

Thanks @invacuo

@invacuo invacuo merged commit 3cebcf3 into rodolfobandeira:master Oct 19, 2018
@invacuo invacuo deleted the add_history_endpoint branch October 19, 2018 12:07
@invacuo
Copy link
Collaborator Author

invacuo commented Oct 19, 2018

I have a WIP branch for implementing filters. It basically uses an optional params hash that then gets passed on to the SPACEX api endpoint. Faraday does the URL encoding for us. I will push it up later today for you to review.

There are some unanswered questions there like what do we do for capsules endpoint that accepts capsule_serial instead of id. Similar questions for a couple of other endpoints.

@invacuo
Copy link
Collaborator Author

invacuo commented Oct 19, 2018

Here you go @rodolfobandeira https://github.com/rodolfobandeira/spacex/compare/master...invacuo:history_with_params?expand=1

@rodolfobandeira
Copy link
Owner

I don't believe deprecating the version 1.0 right now would be a good idea since we just released. Maybe we can create a workaround like adding an extra optional parameter like a hash. For example:

SPACEX::History.info(4, filters = {}) and filters would be something like: {start_at:, end_at:, etc}

Another idea would be just adding different methods and keep .info as it is. This way we add the new filter feature and don't deprecate the current method.

Cheers!

@invacuo
Copy link
Collaborator Author

invacuo commented Oct 20, 2018

Not suggesting deprecating version 1.0 right now. It is just a warning of the upcoming changes in v2 that we will be switching to using keyword arguments instead of positional.

Version 1.0 will work as it is. And it will accept the optional filters hash as you suggested. The code however will also work if someone tries to pass the arguments as keyword arguments.

I also thought about exposing another method which is also not a bad idea. I do like the idea of having just one exposed method better though. What are your thoughts on adding a deprecation warning on .info as we get closer to v2.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodolfobandeira rodolfobandeira rodolfobandeira approved these changes

Assignees

@invacuo invacuo

Labels
ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement History endpoint
3 participants
@invacuo @coveralls @rodolfobandeira